fix: informative errors for None in chunk specifications#4177
Conversation
* fix: byte-order handling for structured dtypes in the bytes codec The bytes codec neither byte-swapped structured-dtype fields to its configured endian on encode (numpy reports byteorder '|' for void dtypes, so the top-level byteorder comparison never detected a mismatch) nor honored its endian when decoding, silently corrupting any structured data whose field byte order differed from the stored one (e.g. virtual references to external big-endian data). Encode now detects byte-order mismatches by comparing full dtypes via newbyteorder, and decode reinterprets raw bytes in the stored byte order before converting to the data type's declared byte order, so the stored layout (codec state) and the in-memory layout (array data type) are independent. Closes zarr-developers#4141 Assisted-by: ClaudeCode:claude-fable-5 * test: fold structured byte-order cases into existing bytes codec tests Extend test_endian's parametrization with structured dtypes and test_bytes_codec_sync_roundtrip with endian/dtype parametrization plus stored-layout and decoded-dtype assertions, instead of adding parallel test functions for the same properties. Assisted-by: ClaudeCode:claude-fable-5 * refactor: rename stored_dtype to view_dtype in BytesCodec decode The variable is the dtype used to view the raw chunk bytes (byte order from the codec's endian configuration), not a property of the stored data or of the returned buffer, which always carries the array's declared dtype. Assisted-by: ClaudeCode:claude-fable-5 * docs: note that the decode-side byte-order conversion copies the chunk Assisted-by: ClaudeCode:claude-fable-5
Two pre-release fixes for chunk normalization error messages in 3.3.0:
- A per-dimension None chunk size (e.g. chunks=(None, 5)), which worked in
3.2.1 as "full extent for this dimension", previously crashed with an
uninformative TypeError ('NoneType' object is not iterable) from
normalize_chunks_1d. It now raises a ValueError directing the user to
the -1 sentinel, as promised by the zarr-developers#3899 release notes entry.
- zarr.create_array(..., chunks=None) raised a self-contradictory message
telling the user to pass chunks=None "from the top-level API". The
message now points at chunks="auto" or omitting the chunks argument.
Assisted-by: ClaudeCode:claude-fable-5
Per review on PR #236, normalize_chunks_1d and normalize_chunks_nd now take object and narrow with explicit isinstance/identity checks instead of growing an ad-hoc union annotation. Behavior changes: - A per-dimension bool chunk size (e.g. chunks=(True, 5)) is now rejected with an informative ValueError. Previously bool being a subclass of int let True through as a silent size-1 chunk — the exact behavior the zarr-developers#3899 release notes say was removed. - Strings/bytes and non-iterable values (e.g. chunks=2.5 or chunks=(2.5, 5)) now raise informative TypeErrors instead of bare crashes (list(2.5), len(generator)) or a misleading dimension-count error for whole-argument strings. - Generator inputs to normalize_chunks_nd are now materialized and accepted, both as the whole argument and as a per-dimension size list in rectilinear specs, consistent with numpy-style APIs. The type: ignore[call-overload] on int(c) is no longer needed after proper narrowing. Assisted-by: ClaudeCode:claude-fable-5
tests/test_api.py::test_create pinned the old bare TypeError message
('float' object is not iterable) that the chunk-normalizer refactor
deliberately replaced with an informative one. Match the new message.
Assisted-by: ClaudeCode:claude-fable-5
…izers The str/bytes rejection and the non-iterable rejection raised identical errors from separate branches in both normalize_chunks_1d and normalize_chunks_nd. Fold each pair into a single condition; str/bytes only need naming because they are iterable. Assisted-by: ClaudeCode:claude-fable-5
normalize_chunks_nd is a mechanical routine and should not refer to chunks="auto", which it does not itself accept. Its None/True rejection now states only what the normalizer expects; the guidance pointing users at chunks="auto" (or omitting the argument) is raised in init_array, the layer where auto-chunking is actually interpreted. Assisted-by: ClaudeCode:claude-fable-5
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4177 +/- ##
=======================================
Coverage 93.84% 93.85%
=======================================
Files 91 91
Lines 12549 12562 +13
=======================================
+ Hits 11777 11790 +13
Misses 772 772
🚀 New features to boost your workflow:
|
Assisted-by: ClaudeCode:claude-fable-5
|
i think this is a candidate for a self-merge. I plan to do so in ~8-16 hours |
| # an object-typed view: None/True are outside ChunksLike but reachable | ||
| # from untyped callers. | ||
| chunks_input: object = chunks | ||
| if chunks_input is None or chunks_input is True: |
There was a problem hiding this comment.
What happens if chunks_input is False?
There was a problem hiding this comment.
Rather than checking for a few invalid values, should the check be if chunks is either "auto", an int, or a tuple of ints?
There was a problem hiding this comment.
What happens if
chunks_input is False?
chunks=false is valid for 2.x compatibility. it creates a single chunk for the whole array 🙃
Rather than checking for a few invalid values, should the check be if chunks is either "auto", an int, or a tuple of ints?
that leaves out iterables of ints, numpy arrays, etc. we have lower-level normalization for that.
| # an object-typed view: None/True are outside ChunksLike but reachable | ||
| # from untyped callers. | ||
| chunks_input: object = chunks | ||
| if chunks_input is None or chunks_input is True: |
There was a problem hiding this comment.
| if chunks_input is None or chunks_input is True: | |
| if not ( | |
| chunks_input == "auto" | |
| or (isinstance(chunks_input, int) and not isinstance(chunks_input, bool)) | |
| or (isinstance(chunks_input, tuple) and all(isinstance(v, int) and not isinstance(v, bool) for v in chunks_input)) | |
| ): |
Perhaps we can actually verify if the value is valid or not
There was a problem hiding this comment.
Claude suggested something like this.
def _is_valid_chunks(value):
if value == "auto":
return True
if isinstance(value, int) and not isinstance(value, bool):
return True
if isinstance(value, tuple) and all(
isinstance(v, int) and not isinstance(v, bool) for v in value
):
return True
return False
if not _is_valid_chunks(chunks_input):
raise ValueError(
f'{chunks_input!r} is not a valid chunk input. Use chunks="auto" or omit the chunks '
"argument for automatic chunking, or pass an int / tuple of ints."
)There was a problem hiding this comment.
we already have type narrowing routines for this. we just need to handle "auto" and deprecated values before calling those functions
There was a problem hiding this comment.
Pull request overview
This PR hardens chunk specification normalization and improves user-facing error messages when invalid chunks values are provided (notably None and boolean-like inputs), aligning the normalizers with a “wide input / narrow output” pattern and ensuring API-level guidance is emitted from the top-level creation helpers.
Changes:
- Make
normalize_chunks_1d/normalize_chunks_ndacceptobjectinputs and raise more informative exceptions for invalid chunk specifications (e.g.,None,bool, strings, non-iterables). - Shift
chunks=None/chunks=Trueguidance toinit_arrayso API users are directed tochunks="auto"/ omittingchunks. - Expand and update tests and release notes to cover the new error semantics and generator acceptance.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_chunk_grids.py | Adds coverage for informative error messages and additional accepted/rejected chunk forms (incl. generators). |
| tests/test_api.py | Updates API-level expectation for chunk-size errors to match the new message. |
| src/zarr/core/chunk_grids.py | Refactors chunk normalization routines to accept object and improve type narrowing + error reporting. |
| src/zarr/core/array.py | Raises API-level ValueError for chunks=None/True with guidance toward auto-chunking. |
| docs/release-notes.md | Documents the more informative chunk-normalization behavior and generator support. |
Comments suppressed due to low confidence (1)
src/zarr/core/chunk_grids.py:752
if chunks == -1runs before establishing thatchunksis a scalar. Ifchunksis an array-like iterable (e.g., a NumPy array),chunks == -1produces an array and theiftriggersValueError: truth value of an array is ambiguousinstead of normalizing via the iterable path.
if chunks == -1:
return np.array([span], dtype=np.int64)
if isinstance(chunks, int):
if chunks <= 0:
raise ValueError(f"Chunk size must be positive, got {chunks}")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Documents that chunks=False (one whole-array chunk, v2 compat), list specs, and numpy integer scalars are valid create_array inputs, in response to review discussion on the init_array None/True guard. Assisted-by: ClaudeCode:claude-fable-5
Summary
Passing
Noneas achunksparameter results in an uninformativeTypeError, when it should raise an informativeValueErrorinstead. This PR makes that change, along with some improvements to the signature of the chunk normalization routines. These routines are now shaped like regular normalization functions, with a wide input type (object) and a narrow output type.These changes were written by claude.
See d-v-b#236 for the original claude-authored PR.
Author attestation
TODO
docs/user-guide/*.mdchanges/